Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(material/core): resolve memory leak by removing event listeners from the ripple element #24663

Merged
merged 1 commit into from
Mar 27, 2022
Merged

Conversation

arturovt
Copy link
Contributor

We were adding transitionend and transitioncancel event listeners, but never removed it. I didn't want to touch the RippleRef class since that would be a change to the public API.

@arturovt arturovt requested a review from devversion as a code owner March 25, 2022 04:54
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this actually is a memory leak? I was thinking about it when adding the event listeners but it seemed fine in profiling and GC.

#24482 (comment)

@arturovt
Copy link
Contributor Author

arturovt commented Mar 25, 2022

Hey Paul, I've double checked it in other browser (Firefox) and the memory leak exists (note I've renamed div to app-ripple through const ripple = document.createElement('app-ripple'); ripple.classList.add('mat-ripple-element'). So it's just easier to find the DOM node in the snapshot):

Снимок экрана 2022-03-25 в 19 01 09

The object ID in the tree is 0x1246115000. Now, let's query the snapshot to find an actual "reference holder":

Снимок экрана 2022-03-25 в 19 06 08

The capture map shows that the DOM node is being captured by lambda (which is () => this._finish....).

That wouldn't be a memory leak in Chrome (because V8's garbage collector is really smart about reference counting) if the lambda didn't capture this, for instance el.addEventListener('some_event', () => el.remove()).

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. I re-checked and was able to reproduce this. Thanks @arturovt

src/material/core/ripple/ripple-renderer.ts Outdated Show resolved Hide resolved
src/material/core/ripple/ripple-renderer.ts Outdated Show resolved Hide resolved
* Map of currently active ripple references.
* The ripple reference is mapped to its element event listeners.
* The reason why `| null` is used is that event listeners are added only
* when the condition is truthy (see the `_startFadeOutTransition` method).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if _startFadeOutTransition is the best reference here, but thanks for the comment 👍

@devversion devversion added P2 The issue is important to a large percentage of users, with a workaround action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Mar 25, 2022
@andrewseguin andrewseguin added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Mar 27, 2022
@andrewseguin andrewseguin merged commit dbb6dc0 into angular:master Mar 27, 2022
@arturovt arturovt deleted the fix/ripple-leak branch March 27, 2022 16:40
forsti0506 pushed a commit to forsti0506/components that referenced this pull request Apr 3, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants